-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
azurerm_network_security_group
- support for augmented security rules
#781
Conversation
This support was already added to the `azurerm_network_security_rule` resource in PR #692, but it wasn’t added to the `azurerm_network_security_group` resource at that time. It did needed a bit of additional logic to make it work properly in the `azurerm_network_security_group` resource as well, but it all worked out nicely. All related unit- and acceptance tests were successfully run before creating this PR.
properties := network.SecurityRulePropertiesFormat{ | ||
SourcePortRange: &source_port_range, | ||
DestinationPortRange: &destination_port_range, | ||
SourceAddressPrefix: &source_address_prefix, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SourceAddressPrefixes should be expanded too, if not during an update of an existing nsg we will drop enhanced rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain what you mean exactly? I'm afraid I don't understand what you mean by SourceAddressPrefixes should be expanded too
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you missed that I check and set those fields (if needed) as well? See below starting from line 288...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry, I made a mistake during my test, your PR fix that problem with #692... And it works as described 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check, cool...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @svanharmelen
Thanks for this PR - apologies for the delay in reviewing it! I've taken a look through and this LGTM - I've run the tests and they pass too :)
Thanks!
azurerm_network_security_group
- support for augmented security rules
Thanks! And no problem... |
👋 hey @svanharmelen Just to let you know that this has just been released in v1.2.0 of the AzureRM Provider - full details of what's included are available here: https://github.com/terraform-providers/terraform-provider-azurerm/blob/v1.2.0/CHANGELOG.md#120-march-02-2018 Thanks! |
Cool... Thanks for the heads up! |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |
This support was already added to the
azurerm_network_security_rule
resource in PR #692, but it wasn’t added to theazurerm_network_security_group
resource at that time.It did need a bit of additional logic to make it work properly in the
azurerm_network_security_group
resource as well, but it all worked out nicely.All related unit- and acceptance tests were successfully run before creating this PR.